-
Notifications
You must be signed in to change notification settings - Fork 5
Enable Bulk User Creation for Larger Batches of Users #597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
fspeirs
wants to merge
16
commits into
main
Choose a base branch
from
batch-user-creation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We require contributors to sign our Contributor License Agreement, and we don't have you on file. In order for us to review and merge your code, please complete this form and we'll get you added and review your contribution as soon as possible. |
danhalson
requested changes
Sep 23, 2025
spec/features/school_student/creating_a_batch_of_school_students_spec.rb
Show resolved
Hide resolved
fspeirs
commented
Sep 29, 2025
fspeirs
commented
Sep 29, 2025
3f1625e
to
783ea8e
Compare
This commit allows ProfileApiClient to call the new fast validation endpoint in Profile.
This is deliberately a serial queue.
This commit creates a new operation that validates a batch. It moves all of the validation error handling out of ::CreateBatch, leaving that operation quite simple.
This commmit takes the validation and concurrency control out of create_students_job and puts it in school_students_controller. The idea is that instead of validating then committing one job of 50 students, we: 1. validate the entire batch of N students quickly by calling SchoolStudents::ValidateBatch 2. Split the batch into chunks of 50. 3. Enqueue them in the context of a GoodJob::Batch, ensuring atomic enqueue of all `N/50` jobs. 4. Control concurrency by not allowing creation of another Batch whose description field matches the school ID (this makes up for the fact that GoodJob::Batch does not have a concurrency key like Jobs do).
Switching to `filter_map` ensures that a parameter list of `[]` gets through as `[]` and not `[nil]`.
The validation functionality has been refactored and raised to the controller level that calls CreateBatch, so the tests are at a higher level now.
Recent updates in profile made a small number of changes to the structure of errors and parameters in profile. This commit brings editor-api up to match it.
This commit moves the method SchoolStudentsController#batch_in_progress? to School#import_in_progress? and changes the batch identifier from "school_id:#{school.id}" to simply "#{school.id}". This allows us to expose the import_in_progress field through the API so that we can enable/disable front-end UI components to prevent user frustration.
This change includes the state of current imports for a school in the API response.
This commit changes UserJob from having a 1:1 relationship with GoodJob::Job to tracking the ID of a GoodJob::Batch. This means that the UserJobsController had to change to track the state of a batch and emulate the status messages that a Job used to provide (e.g. runnning, queued, etc.). The front end can now poll for the status of this job.
We were only decrypting students at validation and not creation. Fix.
783ea8e
to
5204b95
Compare
(Rebased on latest main this morning to resolve conflicts.) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Status
This user story is in Experience CS #1146 - Adding Students - Increase Upload Limit.
The overall goal of this PR is to allow Code Editor to ingest CSV uploads of new users much larger than the current limit of 50 that is imposed by Profile. However, it was also a goal NOT to change the 50-row limit in the
createStudents
endpoint in Profile.Therefore the high level strategy to solving this problem is:
This PR depends on the branch in PR #1948 in Profile and PR #628 in editor-standalone, which removes the 50-row limit check on the front end.
Points for consideration:
Please could reviewers focus on concurrency safety and any issues that could arise if a batch takes a long time to run?
What's changed?
ProfileApiClient
to use the new/preflight-student-upload
endpoint in Profile.SchoolStudent::CreateBatch
and instead validates the entire upload using the newSchoolStudents::ValidateBatch
operation.GoodJob::Batch
to group individualCreateStudentsJob
and ensure that the enqueue of the entire batch of 50-user creation jobs is atomic.description
field of aGoodJob::Batch
as a kind of concurrency key, since batches don't have concurrency keys. The controller will refuse to enqueue a new batch if there exists a batch whose description matches the school ID and which has not been completed or discarded.Steps to perform after deploying to production
I don't believe deploying this PR will require any additional work.